feat(repo-server): Declare custom trust certs for repo-server and plugins #1876
Conversation
olivergondza
changed the title
|
Compared to the proposal in #1876, it turned out 1 init container is enough. Also, this implements |
|
/ok-to-test |
olivergondza
force-pushed
the
ClusterTrustBundle
branch
from
9e0a19d to
4653fab
Compare
olivergondza
changed the title
|
The "Code scans / Run golangci-lint and gosec (pull_request)" failure to be adressed by #1880 |
|
The test failures are related to the fact the code depends on a tech-preview features. Any advise on how to handle such functionality? |
olivergondza
force-pushed
the
ClusterTrustBundle
branch
6 times, most recently
from
93a8f75 to
2e5afb6
Compare
olivergondza
force-pushed
the
ClusterTrustBundle
branch
2 times, most recently
from
de515ac to
448e641
Compare
olivergondza
force-pushed
the
ClusterTrustBundle
branch
6 times, most recently
from
c0a9ad2 to
448e641
Compare
… or plugins Signed-off-by: Oliver Gondža <[email protected]>
…plugins from Secrets and CMs Signed-off-by: Oliver Gondža <[email protected]>
Signed-off-by: Oliver Gondža <[email protected]>
Signed-off-by: Oliver Gondža <[email protected]>
Signed-off-by: Oliver Gondža <[email protected]>
Signed-off-by: Oliver Gondža <[email protected]>
Signed-off-by: Oliver Gondža <[email protected]>
Signed-off-by: Oliver Gondža <[email protected]>
Signed-off-by: Oliver Gondža <[email protected]>
Signed-off-by: Oliver Gondža <[email protected]>
Signed-off-by: Oliver Gondža <[email protected]>
… CTB support Signed-off-by: Oliver Gondža <[email protected]>
Signed-off-by: Oliver Gondža <[email protected]>
Signed-off-by: Oliver Gondža <[email protected]>
olivergondza
force-pushed
the
ClusterTrustBundle
branch
2 times, most recently
from
dfe7ca7 to
198d41d
Compare
…rustBundles to repo-server automatically Signed-off-by: Oliver Gondža <[email protected]>
olivergondza
force-pushed
the
ClusterTrustBundle
branch
from
198d41d to
9c3639c
Compare
controllers/argocd/repo_server.go
Outdated
| MountPath: "/etc/ssl/certs/", | ||
| }, | ||
| }, | ||
| Resources: getArgoRepoResources(cr), |
There was a problem hiding this comment.
Repo server resources are generally quite high. For init container, which is just updating the certs, this can be reduced IMO.
There was a problem hiding this comment.
Definitely. Though I see copyutil init container is using the same.
I am thinking if this is not actually desirable to occupy the resources early in the container start because a) the prod container is going to occupy the resources anyway, and b) init container are executed serially.
…rest - Use default image pull policy - Use default security context - Propagate proxy env vars Signed-off-by: Oliver Gondža <[email protected]>
|
@anandf, thanks for the review. I have addressed all expect for the resources, as the discussion there is ongoing... |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe PR introduces system CA trust support to Argo CD operators, enabling administrators to inject custom CA certificates into repo-server containers via ClusterTrustBundles, Secrets, or ConfigMaps. Changes include API schema extensions, controller logic for volume projection and init containers, resource watchers for dynamic updates, comprehensive E2E tests, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
actor Admin as Administrator
participant API as Kubernetes API
participant Ctrl as Argo CD Controller
participant CTB as ClusterTrustBundle
participant CM as ConfigMap/Secret
participant Pod as Repo-Server Pod
Admin->>API: Create/Update ArgoCD with systemCATrust config
Ctrl->>Ctrl: Detect change via systemCATrustMapper
Ctrl->>CTB: Watch & resolve ClusterTrustBundles
Ctrl->>CM: Watch & resolve Secrets/ConfigMaps
Ctrl->>Ctrl: Build volumes & init container spec
rect rgb(220, 240, 255)
note over Ctrl,Pod: Volume Projection Phase
Ctrl->>Pod: Mount ca-trust-source volume<br/>(ClusterTrustBundles+Secrets+ConfigMaps)
end
rect rgb(240, 220, 255)
note over Ctrl,Pod: Init Container Phase
Ctrl->>Pod: Run update-ca-certificates init container<br/>Merges image certs with projected sources<br/>Output to ca-trust-target (emptyDir)
end
rect rgb(220, 255, 220)
note over Ctrl,Pod: Production Phase
Ctrl->>Pod: Mount ca-trust-target to /etc/ssl/certs/<br/>Mount ca-trust-source to /usr/local/share/ca-certificates/
Pod->>Pod: Repo-server uses unified CA bundle
end
Admin->>API: Update Secret/ConfigMap/ClusterTrustBundle
Ctrl->>Ctrl: Detect change via resourceMapper
Ctrl->>Pod: Trigger pod restart
sequenceDiagram
participant isRelevant as isRelevantCtb()
participant Proj as Projection Config
participant CTB as Actual ClusterTrustBundle
isRelevant->>Proj: Extract name or signerName + labelSelector
isRelevant->>CTB: Fetch from cluster
alt Name-based matching
isRelevant->>Proj: Get projection.name
isRelevant->>CTB: Get ctb.metadata.name
isRelevant->>isRelevant: name == name?
else SignerName + Label Matching
isRelevant->>CTB: Get ctb.spec.signerName
isRelevant->>Proj: Get projection.signerName
isRelevant->>isRelevant: signerName match?
isRelevant->>CTB: Get ctb.metadata.labels
isRelevant->>Proj: Get projection.labelSelector
rect rgb(255, 240, 220)
note over isRelevant: Evaluate label selector against CTB labels
end
end
isRelevant-->>isRelevant: Return true/false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/ginkgo/fixture/application/fixture.go (1)
109-120: Close the backtick in the diagnostic string.
fmt.Sprintf(" -%s/%s", …)` prints an unmatched backtick; adding the closing backtick keeps the debug output balanced and a bit clearer.- found = append(found, fmt.Sprintf(" - `%s/%s", condition.Type, condition.Message)) + found = append(found, fmt.Sprintf(" - `%s/%s`", condition.Type, condition.Message))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/ci-build.yaml(2 hunks)api/v1beta1/argocd_types.go(2 hunks)api/v1beta1/zz_generated.deepcopy.go(2 hunks)bundle/manifests/argoproj.io_argocds.yaml(1 hunks)config/crd/bases/argoproj.io_argocds.yaml(1 hunks)controllers/argocd/argocd_controller.go(1 hunks)controllers/argocd/repo_server.go(5 hunks)controllers/argocd/util.go(8 hunks)deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml(1 hunks)docs/reference/argocd.md(2 hunks)tests/ginkgo/fixture/application/fixture.go(2 hunks)tests/ginkgo/fixture/argocd/fixture.go(1 hunks)tests/ginkgo/fixture/pod/fixture.go(2 hunks)tests/ginkgo/fixture/utils/fixtureUtils.go(2 hunks)tests/ginkgo/sequential/1-120_repo_server_system_ca_trust.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/reference/argocd.md
[uncategorized] ~1137-~1137: Possible missing article found.
Context: ... - The certificates are not pinned for individual host, so CA/wildcard certificates can b...
(AI_HYDRA_LEO_MISSING_AN)
🪛 markdownlint-cli2 (0.18.1)
docs/reference/argocd.md
1139-1139: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
1140-1140: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
1141-1141: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (11)
.github/workflows/ci-build.yaml (3)
51-53: Matrix strategy correctly balances version coverage for ClusterTrustBundle support.Introducing a two-version matrix (v1.27.1-k3s1 and v1.33.5-k3s1) appropriately addresses the dual requirement: maintaining Kubernetes 1.27 compatibility for downstream users while enabling tech-preview features for newer versions.
74-86: Verify version regex pattern captures all intended versions for ClusterTrustBundle support.The conditional feature-flag logic looks correct, but the regex pattern
v1.3[3456789]*matches only v1.33 and later (v1.34–v1.39+), excluding v1.30, v1.31, and v1.32. If ClusterTrustBundle is available and supported in those earlier 1.3x versions, the pattern should be expanded tov1.3[0-9]*to avoid gaps in coverage.Please confirm whether:
- ClusterTrustBundle is only available/tested in k3s 1.33+, making the current pattern intentional?
- Or should the pattern be
v1.3[0-9]*to capture 1.30–1.39+ uniformly?If the latter, apply this diff:
- v1.3[3456789]*) + v1.3[0-9]*)
74-86: Feature flag configuration is syntactically correct and well-documented.The use of a bash array with proper expansion
"${feature_flags[@]}"and clear inline comments explaining why the flags are needed is idiomatic and maintainable. The flags correctly enable both ClusterTrustBundle and ClusterTrustBundleProjection, plus the required runtime-config, aligning with the PR objectives.tests/ginkgo/fixture/utils/fixtureUtils.go (1)
126-128: Good catch registering the certificates API.Adding
certificatesv1beta1to the scheme prevents runtime decoding errors once the tests start creating or readingClusterTrustBundleresources.tests/ginkgo/fixture/pod/fixture.go (1)
17-31: Nice helper for regex pod lookup.The focused matcher keeps the tests tidy and the
HaveLen(1)assertion guards the single-match invariant up front.deploy/olm-catalog/argocd-operator/0.17.0/argoproj.io_argocds.yaml (3)
20259-20263: Clarify dropImageCertificates semantics and defaults.The
dropImageCertificatesfield (line 20259-20263) lacks a default value specification. Clarify in documentation:
- What is the default behavior if omitted? (Presumably false, preserving image certs)
- Should this default be explicitly set to
falsein the schema for clarity?- How does this interact with the merging of system CAs?
This is a breaking behavior change if not properly documented. Consider adding:
dropImageCertificates: description: DropImageCertificates will remove all certs that are present in the image, leaving only those explicitly configured here. + default: false type: boolean
20101-20331: Based on my verification attempts, the external documentation and release notes do not provide specific information about thesystemCATrustfeature. The previous script execution was truncated due to buffer limitations. Given this, I'll provide the rewritten review comment based on the evidence gathered:Verify CRD schema is properly synchronized with corresponding Go type definitions in the codebase.
The systemCATrust schema extension for repo server certificate management appears to follow Kubernetes conventions correctly. The YAML defines a well-formed structure with proper Kubernetes markers (x-kubernetes-list-type, x-kubernetes-map-type) and includes four sub-properties: clusterTrustBundles, configMaps, dropImageCertificates, and secrets. However, direct verification with the Go type definitions (
ArgoCDSystemCATrustSpecand related types) is needed to confirm this CRD manifest was properly code-generated and remains synchronized. This is particularly important given that auto-generation from Go types can diverge if the operator's Go source was not properly rebuilt after recent changes.
20177-20190: No verification needed; path field semantics are correctly implemented.The ClusterTrustBundleProjection.path field must be a relative path (relative to the volume mount), may not be absolute, and may not contain or start with ".."—these constraints are enforced by the Kubernetes API server. The YAML at lines 20177-20190 correctly marks
pathas required for ClusterTrustBundleProjection items, reflecting the standard Kubernetes type definition. The operator's code additionally callsensureValidPath()incontrollers/argocd/repo_server.go(line 675) for supplementary validation. All path requirements are already enforced and no changes are required.bundle/manifests/argoproj.io_argocds.yaml (1)
20101-20331: Schema generation verified—all fields correctly match Go type definitions.The CRD schema is properly auto-generated from the Go types:
- All 4 fields from
ArgoCDSystemCATrustSpecare present in the manifest- Field names and types align exactly (ClusterTrustBundles, ConfigMaps, Secrets, DropImageCertificates)
- Uses standard Kubernetes projection types (corev1.ClusterTrustBundleProjection, corev1.SecretProjection, corev1.ConfigMapProjection)
- All fields correctly marked as optional (matching
omitemptyin Go struct tags)- No schema errors or discrepancies detected
config/crd/bases/argoproj.io_argocds.yaml (2)
20090-20320: Schema structure looks sound; verify mutual-exclusivity validation is enforced in code.The systemCATrust schema properly defines volume projections for injecting CA certificates into repo-server. The structure, types, and Kubernetes-specific annotations (x-kubernetes-list-type, x-kubernetes-map-type) are correct. However, the ClusterTrustBundleProjection's mutual-exclusivity constraints (lines 20154-20156 and 20172-20174: name vs. signerName/labelSelector) are documented in descriptions but cannot be enforced at the CRD schema level. These constraints must be validated in the Go controller code.
To confirm mutual-exclusivity and other validation constraints are properly handled, verify that the controller code includes validation logic for these fields.
20248-20252: Clarify field name: dropImageCertificates vs. dropImageAnchors.Per the PR comments, the feature was referred to as "DropImageAnchors," but the schema field is named "dropImageCertificates." Confirm this naming is intentional and consistent across the codebase (API types, controller, tests, documentation).
|
|
||
| return corev1.Container{ | ||
| Name: "update-ca-certificates", | ||
| Image: argoImage, | ||
| ImagePullPolicy: argoutil.GetImagePullPolicy(cr.Spec.ImagePullPolicy), | ||
| Env: proxyEnvVars(corev1.EnvVar{ | ||
| Name: "IMAGE_CERT_PATH", | ||
| Value: imageCertPath, | ||
| }), | ||
| Command: []string{"/bin/bash", "-c"}, | ||
| Args: []string{` | ||
| set -eEuo pipefail | ||
| trap 's=$?; echo >&2 "$0: Error on line "$LINENO": $BASH_COMMAND"; exit $s' ERR | ||
| echo "User defined CA files:" | ||
| ls -l /usr/local/share/ca-certificates/ | ||
| # Make sure the file exist even when the update-ca-certificates produces no pem blocks | ||
| echo "" > /etc/ssl/certs/ca-certificates.crt | ||
| update-ca-certificates --verbose --certsdir "$IMAGE_CERT_PATH" | ||
| echo "Resulting /etc/ssl/certs/" | ||
| ls -l /etc/ssl/certs/ | ||
| echo "Done!" | ||
| `}, | ||
| VolumeMounts: []corev1.VolumeMount{ | ||
| { | ||
| Name: volumeSource, | ||
| // Source path for user additional certificates - empty in the image, so not shadowing anything. | ||
| MountPath: "/usr/local/share/ca-certificates/", | ||
| ReadOnly: true, | ||
| }, { | ||
| Name: volumeTarget, | ||
| MountPath: "/etc/ssl/certs/", | ||
| }, { | ||
| Name: "tmp", | ||
| MountPath: "/tmp", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Respect user-supplied /tmp volume mounts in the init container
When an operator override mounts /tmp through spec.repo.volumeMounts, we skip creating the default "tmp" volume—but caTrustInitContainer still hardcodes a VolumeMount{Name: "tmp"}. The resulting pod fails admission because the referenced volume doesn’t exist. Please reuse the existing /tmp mount (or omit it if none is available) when building the init container’s volume mounts so customized deployments still start.
-func caTrustInitContainer(cr *argocdoperatorv1beta1.ArgoCD, argoImage string, volumeSource string, volumeTarget string) corev1.Container {
+func caTrustInitContainer(cr *argocdoperatorv1beta1.ArgoCD, argoImage string, volumeSource string, volumeTarget string, tmpVolumeName string) corev1.Container {
@@
- VolumeMounts: []corev1.VolumeMount{
+ VolumeMounts: append([]corev1.VolumeMount{
{
Name: volumeSource,
// Source path for user additional certificates - empty in the image, so not shadowing anything.
MountPath: "/usr/local/share/ca-certificates/",
ReadOnly: true,
}, {
Name: volumeTarget,
MountPath: "/etc/ssl/certs/",
- }, {
- Name: "tmp",
- MountPath: "/tmp",
- },
- },
+ },
+ }, func() []corev1.VolumeMount {
+ if tmpVolumeName == "" {
+ return nil
+ }
+ return []corev1.VolumeMount{{
+ Name: tmpVolumeName,
+ MountPath: "/tmp",
+ }}
+ }()...),And update the call site to pass the resolved tmp volume name (or "" if none is available).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In controllers/argocd/repo_server.go around lines 705 to 743, the
caTrustInitContainer unconditionally adds a VolumeMount with Name: "tmp" which
breaks pods when an operator override supplies a different /tmp mount name (or
omits the default tmp volume). Change the init container construction to accept
a resolved tmp volume name (empty string if none) and only append a /tmp
VolumeMount when that name is non-empty; if a tmp name is provided, use it as
the VolumeMount.Name, otherwise omit the mount entirely. Also update the call
site to compute and pass the actual tmp volume name (or "" when absent) derived
from spec.repo.volumeMounts so custom /tmp mounts are respected.
There was a problem hiding this comment.
As per reconcileRepoDeployment, the /tmp is always there. Either user-provided, or the operator default one.
There was a problem hiding this comment.
@olivergondza, you're right that the /tmp mount path is always present—either from the default or user-provided configuration. However, CodeRabbit's concern is about the volume name, not the mount path.
Looking at the logic in reconcileRepoDeployment:
- When
volumeMountOverridesTmpVolumeistrue(user provides a mount withMountPath: "/tmp"), the default volume withName: "tmp"is not created - The user's custom volumes are then appended, which could have any name (e.g.,
"my-custom-tmp") - The init container in
caTrustInitContainerhardcodesName: "tmp"in its VolumeMount
Scenario where this breaks:
spec:
repo:
volumes:
- name: my-custom-tmp
emptyDir: {}
volumeMounts:
- name: my-custom-tmp
mountPath: /tmpIn this case:
- No volume named
"tmp"exists (only"my-custom-tmp") - The init container references
Name: "tmp"→ pod admission fails
The main containers handle this correctly because user-provided volumeMounts are appended and can override. The init container doesn't have this flexibility.
Question: Is the expectation that users must always name their custom /tmp volume as "tmp"? If so, this should be documented. Otherwise, we should fix the init container to use the correct volume name dynamically.
There was a problem hiding this comment.
@anandf, Rabbit here is correct ☝️. I did depend on existence of volume named tmp - that is not guaranteed. We can either:
- Get back to writable
/ - Reuse pod temp volume, no matter what its name is.
- Use dedicated empty volume as
/tmpfor this init container
Any preference?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
3 participants
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
Permit users to trust CAs on a repo-server system level
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes #1830
How to test changes / Special notes to the reviewer:
Can be tested against:
Summary by CodeRabbit
New Features
Documentation